Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes an error thrown when assigning to readOnly property 'toJSON' #1293

Closed
wants to merge 1 commit into from

Conversation

capaj
Copy link

@capaj capaj commented Sep 7, 2015

this breaks my app when I try to create a document which is a dupe according to my compound index

…erty 'toJSON'

this breaks my app when I try to create a document which is a dupe according to my compound index
@capaj
Copy link
Author

capaj commented Sep 7, 2015

those test failures don't seem like something I could have broken with my little change

@christkv
Copy link
Contributor

christkv commented Sep 8, 2015

can you please add a test case that causes this to fail with the current driver, this will make it easier for me to accept this pull request.

@christkv
Copy link
Contributor

Still missing a test case that causes this to happen. Unfortunately I'm not going to merge this until I understand why you are experiencing this problem while there has been no other such issues from other users.

@capaj
Copy link
Author

capaj commented Sep 14, 2015

@christkv thanks for reminding me. I was busy over the weekend, will try to make a test case in the next couple of days. No rush on merging it. Weird that others don't experience the same problems.

@christkv
Copy link
Contributor

closing due to no response

@christkv christkv closed this Oct 28, 2015
@andrewaarestad
Copy link

please merge this, I'm seeing this problem too.

@christkv
Copy link
Contributor

please provide a test that reproduces this issue

@capaj
Copy link
Author

capaj commented Dec 19, 2015

@andrewaarestad this was happening for me because I have used a package https://www.npmjs.com/package/error-tojson which extended Error.prototype.
If you're seeing this issue, you must be extending it somewhere. Get rid of that code. Extending Error.prototype is not a very good thing to do, even if Mongo was't crashing on it.

@capaj
Copy link
Author

capaj commented Dec 19, 2015

@christkv writing a repro test case would not be hard- just https://github.com/AVVS/error-tojson/blob/master/index.js before doing anything with mongo which returns an error. Is it worth it?
Maybe just a mention into the docs to be aware of extending Error.prototype would maybe suffice, no?

P.S.: I've created a note on readme for error-tojson: AVVS/error-tojson#1

@christkv
Copy link
Contributor

Writing it in the docs won't make a difference. I think you are on your own when monkey patching core JS classes. It's very much a no no thing to do as it causes unpredictable behavior in any package depending on it.

@meefik
Copy link
Contributor

meefik commented Mar 15, 2016

Installed modules:

  • mongoose: 4.4.7
  • mongodb: 2.1.7
2016-03-15T19:06:04.222Z - error: uncaughtException: Cannot assign to read only property 'toJSON' of MongoError: E11000 duplicate key error index: mydb.users.$username_1 dup key: { : "student1" } date=Tue Mar 15 2016 19:06:04 GMT+0000 (UTC), pid=13897, uid=1000, gid=1000, cwd=/home/ubuntu/workspace, execPath=/home/ubuntu/.nvm/versions/node/v4.1.1/bin/node, version=v4.1.1, argv=[/home/ubuntu/.nvm/versions/node/v4.1.1/bin/node, /home/ubuntu/workspace/bin/www], rss=171732992, heapTotal=75247104, heapUsed=56467360, loadavg=[9.0712890625, 5.744140625, 4.748046875], uptime=534987
TypeError: Cannot assign to read only property 'toJSON' of MongoError: E11000 duplicate key error index: mydb.users.$username_1 dup key: { : "student1" }
    at toError (/home/ubuntu/workspace/node_modules/mongoose/node_modules/mongodb/lib/utils.js:123:16)
    at /home/ubuntu/workspace/node_modules/mongoose/node_modules/mongodb/lib/bulk/unordered.js:469:64
    at handleCallback (/home/ubuntu/workspace/node_modules/mongoose/node_modules/mongodb/lib/utils.js:96:12)
    at resultHandler (/home/ubuntu/workspace/node_modules/mongoose/node_modules/mongodb/lib/bulk/unordered.js:417:5)
    at commandCallback (/home/ubuntu/workspace/node_modules/mongoose/node_modules/mongodb/node_modules/mongodb-core/lib/topologies/server.js:936:9)
    at Callbacks.emit (/home/ubuntu/workspace/node_modules/mongoose/node_modules/mongodb/node_modules/mongodb-core/lib/topologies/server.js:116:3)
    at null.messageHandler (/home/ubuntu/workspace/node_modules/mongoose/node_modules/mongodb/node_modules/mongodb-core/lib/topologies/server.js:282:23)
    at Socket.<anonymous> (/home/ubuntu/workspace/node_modules/mongoose/node_modules/mongodb/node_modules/mongodb-core/lib/connection/connection.js:273:22)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:169:7)
    at readableAddChunk (_stream_readable.js:146:16)
    at Socket.Readable.push (_stream_readable.js:110:10)
    at TCP.onread (net.js:523:20)

Fixes in "mongodb/lib/utils.js" (use try ... catch):

/**
 * Wrap a Mongo error document in an Error instance
 * @ignore
 * @api private
 */
var toError = function(error) {
  if (error instanceof Error) return error;

  var msg = error.err || error.errmsg || error.errMessage || error;
  var e = MongoError.create({message: msg, driver:true});

  // Get all object keys
  var keys = typeof error == 'object'
    ? Object.keys(error)
    : [];

  for(var i = 0; i < keys.length; i++) {
    try {
      e[keys[i]] = error[keys[i]];
    } catch(err) {
      // continue
    }
  }

  return e;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants